Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use GitHub Actions for linting and testing as well as update gemspec #71

Merged
merged 10 commits into from
Mar 9, 2024

Conversation

erickguan
Copy link
Owner

@erickguan erickguan commented Mar 2, 2024

Add GitHub workflows for linting and testing.

This PR also uses rubocop and update a few gemspec defintions.

It would be nice to move to minitest.

@erickguan erickguan force-pushed the pipeline branch 13 times, most recently from a438249 to cfaae6f Compare March 2, 2024 20:05
@erickguan erickguan requested a review from damiann March 2, 2024 20:06
@erickguan erickguan marked this pull request as ready for review March 2, 2024 20:06
@erickguan erickguan changed the title Use GitHub Actions and update old parts of gem Use GitHub Actions for linting and testing as well as update gemspec Mar 2, 2024
merge:
- Exclude

require:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe later we can add rubocop-performance

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

Comment on lines +29 to +32
Style/StringLiterals:
Enabled: true
EnforcedStyle: single_quotes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default, is your intention here to be explicit on these style decisions?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I agree that we should use as many defaults as possible. For some rules that have a large "surface", I would mark them explicitly.

.rubocop.yml Outdated
# Enforce comma after the last item of a multiline array or hash
Style/TrailingCommaInArrayLiteral:
Enabled: true
EnforcedStyleForMultiline: comma
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern doesn't follow the recommended Ruby Programming Style. I would recommend following up to use default styling for this, Style/TrailingCommaInHashLiteral, and Style/TrailingCommaInArguments.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense.

EnforcedStyle: trailing

Layout/LineLength:
Max: 120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default value. Recommend removing things that are default to reduce config size.

.rubocop.yml Outdated

# Allow guard clauses
Style/GuardClause:
Enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabled by default.

.rubocop.yml Outdated

# Naming conventions
Naming/PredicateName:
Enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabled by default.

.rubocop.yml Outdated

Metrics/BlockLength:
Max: 50

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change


Example:

```ruby
ICU::Transliteration.transliterate('Traditional-Simplified', '沈從文') # => "沈从文"
```

Locale
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these examples?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I made a mistake.

Comment on lines 186 to 192
# rubocop:disable Style/OptionalBooleanParameter
def to_language_tag(strict = false)
Lib::Util.read_string_buffer(64) do |buffer, status|
Lib.uloc_toLanguageTag(@id, buffer, buffer.size, strict ? 1 : 0, status)
end
end
# rubocop:enable Style/OptionalBooleanParameter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These enable/disable cop comments it can also be done on one line.

Suggested change
# rubocop:disable Style/OptionalBooleanParameter
def to_language_tag(strict = false)
Lib::Util.read_string_buffer(64) do |buffer, status|
Lib.uloc_toLanguageTag(@id, buffer, buffer.size, strict ? 1 : 0, status)
end
end
# rubocop:enable Style/OptionalBooleanParameter
def to_language_tag(strict = false) # rubocop:disable Style/OptionalBooleanParameter
Lib::Util.read_string_buffer(64) do |buffer, status|
Lib.uloc_toLanguageTag(@id, buffer, buffer.size, strict ? 1 : 0, status)
end
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with some other files.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -1,3 +1,3 @@
module ICU
VERSION = "0.5.3"
VERSION = '0.5.3'.freeze
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the # frozen_string_literal: true comment could help. Related cop.

@erickguan erickguan requested a review from damiann March 5, 2024 20:08
Copy link
Collaborator

@damiann damiann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small fix, but overall looks really good 👍🏼

.rubocop.yml Outdated
Include:
- '*.gemspec'

# ALlow no documentation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ALlow no documentation.
# Allow no documentation.

@erickguan erickguan force-pushed the pipeline branch 2 times, most recently from 21be71e to e72e108 Compare March 9, 2024 16:15
@erickguan erickguan force-pushed the pipeline branch 3 times, most recently from 0d0c513 to 7b0452a Compare March 9, 2024 16:51
@erickguan erickguan merged commit 0ff6306 into master Mar 9, 2024
11 checks passed
@erickguan erickguan deleted the pipeline branch March 9, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants